<html>
<head><meta charset="utf-8"><title>Review request for #68241 · t-compiler · Zulip Chat Archive</title></head>
<h2>Stream: <a href="https://rust-lang.github.io/zulip_archive/stream/131828-t-compiler/index.html">t-compiler</a></h2>
<h3>Topic: <a href="https://rust-lang.github.io/zulip_archive/stream/131828-t-compiler/topic/Review.20request.20for.20.2368241.html">Review request for #68241</a></h3>

<hr>

<base href="https://rust-lang.zulipchat.com">

<head><link href="https://rust-lang.github.io/zulip_archive/style.css" rel="stylesheet"></head>

<a name="187734372"></a>
<h4><a href="https://rust-lang.zulipchat.com#narrow/stream/131828-t-compiler/topic/Review%20request%20for%20%2368241/near/187734372" class="zl"><img src="https://rust-lang.github.io/zulip_archive/assets/img/zulip.svg" alt="view this post on Zulip" style="width:20px;height:20px;"></a> Dylan MacKenzie (ecstatic-morse) <a href="https://rust-lang.github.io/zulip_archive/stream/131828-t-compiler/topic/Review.20request.20for.20.2368241.html#187734372">(Feb 08 2020 at 22:47)</a>:</h4>
<p><a href="https://github.com/rust-lang/rust/issues/68241" target="_blank" title="https://github.com/rust-lang/rust/issues/68241">#68241</a> been blocked on review from <span class="user-mention" data-user-id="116083">@pnkfelix</span> for a few weeks now. That PR is in turn blocking significant perf improvements in <a href="https://github.com/rust-lang/rust/issues/68528" target="_blank" title="https://github.com/rust-lang/rust/issues/68528">#68528</a>. Would someone be willing to review all or part of <a href="https://github.com/rust-lang/rust/issues/68241" target="_blank" title="https://github.com/rust-lang/rust/issues/68241">#68241</a>? I say "part" because that PR includes several small, independent changes for better perf  and debugging that are closely related to the borrowck dataflow changes but could be split out. Those changes are already isolated into commits. cc <span class="user-group-mention" data-user-group-id="492">@T-compiler</span></p>



<a name="187739261"></a>
<h4><a href="https://rust-lang.zulipchat.com#narrow/stream/131828-t-compiler/topic/Review%20request%20for%20%2368241/near/187739261" class="zl"><img src="https://rust-lang.github.io/zulip_archive/assets/img/zulip.svg" alt="view this post on Zulip" style="width:20px;height:20px;"></a> pnkfelix <a href="https://rust-lang.github.io/zulip_archive/stream/131828-t-compiler/topic/Review.20request.20for.20.2368241.html#187739261">(Feb 09 2020 at 01:30)</a>:</h4>
<p>Yeah I’m sorry, a bunch of my review requests have gone mishandled for far too long</p>



<a name="187739269"></a>
<h4><a href="https://rust-lang.zulipchat.com#narrow/stream/131828-t-compiler/topic/Review%20request%20for%20%2368241/near/187739269" class="zl"><img src="https://rust-lang.github.io/zulip_archive/assets/img/zulip.svg" alt="view this post on Zulip" style="width:20px;height:20px;"></a> pnkfelix <a href="https://rust-lang.github.io/zulip_archive/stream/131828-t-compiler/topic/Review.20request.20for.20.2368241.html#187739269">(Feb 09 2020 at 01:31)</a>:</h4>
<p>It didn’t help that I didn’t get as much free time at the Mozilla all hands as I was expected</p>



<a name="187739311"></a>
<h4><a href="https://rust-lang.zulipchat.com#narrow/stream/131828-t-compiler/topic/Review%20request%20for%20%2368241/near/187739311" class="zl"><img src="https://rust-lang.github.io/zulip_archive/assets/img/zulip.svg" alt="view this post on Zulip" style="width:20px;height:20px;"></a> pnkfelix <a href="https://rust-lang.github.io/zulip_archive/stream/131828-t-compiler/topic/Review.20request.20for.20.2368241.html#187739311">(Feb 09 2020 at 01:32)</a>:</h4>
<p>I won’t get to it until Monday at the earliest</p>



<a name="187743971"></a>
<h4><a href="https://rust-lang.zulipchat.com#narrow/stream/131828-t-compiler/topic/Review%20request%20for%20%2368241/near/187743971" class="zl"><img src="https://rust-lang.github.io/zulip_archive/assets/img/zulip.svg" alt="view this post on Zulip" style="width:20px;height:20px;"></a> Dylan MacKenzie (ecstatic-morse) <a href="https://rust-lang.github.io/zulip_archive/stream/131828-t-compiler/topic/Review.20request.20for.20.2368241.html#187743971">(Feb 09 2020 at 04:23)</a>:</h4>
<p>No worries. Just hoping to find someone who could work steal.</p>



<a name="187766891"></a>
<h4><a href="https://rust-lang.zulipchat.com#narrow/stream/131828-t-compiler/topic/Review%20request%20for%20%2368241/near/187766891" class="zl"><img src="https://rust-lang.github.io/zulip_archive/assets/img/zulip.svg" alt="view this post on Zulip" style="width:20px;height:20px;"></a> Wesley Wiser <a href="https://rust-lang.github.io/zulip_archive/stream/131828-t-compiler/topic/Review.20request.20for.20.2368241.html#187766891">(Feb 09 2020 at 17:53)</a>:</h4>
<p><span class="user-mention" data-user-id="118594">@ecstatic-morse</span> If you split out the first 5 commits into a separate PR, I'd be happy to r+ that. I may be able to review additional parts later as well.</p>



<a name="187768418"></a>
<h4><a href="https://rust-lang.zulipchat.com#narrow/stream/131828-t-compiler/topic/Review%20request%20for%20%2368241/near/187768418" class="zl"><img src="https://rust-lang.github.io/zulip_archive/assets/img/zulip.svg" alt="view this post on Zulip" style="width:20px;height:20px;"></a> Dylan MacKenzie (ecstatic-morse) <a href="https://rust-lang.github.io/zulip_archive/stream/131828-t-compiler/topic/Review.20request.20for.20.2368241.html#187768418">(Feb 09 2020 at 18:42)</a>:</h4>
<p>Thanks <span class="user-mention" data-user-id="125250">@Wesley Wiser</span>! I split the graphviz changes into <a href="https://github.com/rust-lang/rust/issues/69005" target="_blank" title="https://github.com/rust-lang/rust/issues/69005">#69005</a>.</p>



<a name="187768479"></a>
<h4><a href="https://rust-lang.zulipchat.com#narrow/stream/131828-t-compiler/topic/Review%20request%20for%20%2368241/near/187768479" class="zl"><img src="https://rust-lang.github.io/zulip_archive/assets/img/zulip.svg" alt="view this post on Zulip" style="width:20px;height:20px;"></a> Wesley Wiser <a href="https://rust-lang.github.io/zulip_archive/stream/131828-t-compiler/topic/Review.20request.20for.20.2368241.html#187768479">(Feb 09 2020 at 18:44)</a>:</h4>
<p>Reviewed &amp; approved</p>



<a name="187856063"></a>
<h4><a href="https://rust-lang.zulipchat.com#narrow/stream/131828-t-compiler/topic/Review%20request%20for%20%2368241/near/187856063" class="zl"><img src="https://rust-lang.github.io/zulip_archive/assets/img/zulip.svg" alt="view this post on Zulip" style="width:20px;height:20px;"></a> pnkfelix <a href="https://rust-lang.github.io/zulip_archive/stream/131828-t-compiler/topic/Review.20request.20for.20.2368241.html#187856063">(Feb 10 2020 at 20:44)</a>:</h4>
<p><span class="user-mention" data-user-id="118594">@ecstatic-morse</span> just so we're on the same page regarding expectations for this PR : when I followed the perf.rlo link, the the task-clock and wall-times show a fair amount of red (i.e. regressions). They might be acceptable regressions, but reading the description of the PR itself, I wasn't expecting to see much regression at all.</p>



<a name="187856080"></a>
<h4><a href="https://rust-lang.zulipchat.com#narrow/stream/131828-t-compiler/topic/Review%20request%20for%20%2368241/near/187856080" class="zl"><img src="https://rust-lang.github.io/zulip_archive/assets/img/zulip.svg" alt="view this post on Zulip" style="width:20px;height:20px;"></a> pnkfelix <a href="https://rust-lang.github.io/zulip_archive/stream/131828-t-compiler/topic/Review.20request.20for.20.2368241.html#187856080">(Feb 10 2020 at 20:44)</a>:</h4>
<p><span class="user-mention" data-user-id="118594">@ecstatic-morse</span> am I misunderstanding the perf results? Or looking at an outdated link:?</p>



<a name="187857629"></a>
<h4><a href="https://rust-lang.zulipchat.com#narrow/stream/131828-t-compiler/topic/Review%20request%20for%20%2368241/near/187857629" class="zl"><img src="https://rust-lang.github.io/zulip_archive/assets/img/zulip.svg" alt="view this post on Zulip" style="width:20px;height:20px;"></a> Dylan MacKenzie (ecstatic-morse) <a href="https://rust-lang.github.io/zulip_archive/stream/131828-t-compiler/topic/Review.20request.20for.20.2368241.html#187857629">(Feb 10 2020 at 21:01)</a>:</h4>
<p>I expect this PR to be <em>roughly</em> performance neutral, maybe slightly positive: some regressions due to additional boilerplate and a more fine-grained cursor offset by skipping block-transfer function caching.</p>
<p>cc <span class="user-mention" data-user-id="116122">@simulacrum</span> I am assuming that the changes in wall-time measurements were due to variance. For example, here's the <a href="https://perf.rust-lang.org/compare.html?start=4b172cc73f5f1fb1e3d60527fb29605fa267985c&amp;end=31ca0fd7b109756a7c52baff29870d7831f3e723&amp;stat=task-clock" target="_blank" title="https://perf.rust-lang.org/compare.html?start=4b172cc73f5f1fb1e3d60527fb29605fa267985c&amp;end=31ca0fd7b109756a7c52baff29870d7831f3e723&amp;stat=task-clock">task-clock results for an initial version of the PR</a>. This is <em>without</em> the MIR visitor or the block transfer function skipping. I don't think these results represent a true  2% improvement to some opt/debug builds,  even though there is more green. I expect any true perf changes from this PR to be in check builds, where changes in borrowck performance aren't dwarfed by variance during codegen.</p>



<a name="187857686"></a>
<h4><a href="https://rust-lang.zulipchat.com#narrow/stream/131828-t-compiler/topic/Review%20request%20for%20%2368241/near/187857686" class="zl"><img src="https://rust-lang.github.io/zulip_archive/assets/img/zulip.svg" alt="view this post on Zulip" style="width:20px;height:20px;"></a> Dylan MacKenzie (ecstatic-morse) <a href="https://rust-lang.github.io/zulip_archive/stream/131828-t-compiler/topic/Review.20request.20for.20.2368241.html#187857686">(Feb 10 2020 at 21:02)</a>:</h4>
<p>Btw, lemme rebase away the graphviz changes</p>



<a name="187858177"></a>
<h4><a href="https://rust-lang.zulipchat.com#narrow/stream/131828-t-compiler/topic/Review%20request%20for%20%2368241/near/187858177" class="zl"><img src="https://rust-lang.github.io/zulip_archive/assets/img/zulip.svg" alt="view this post on Zulip" style="width:20px;height:20px;"></a> Dylan MacKenzie (ecstatic-morse) <a href="https://rust-lang.github.io/zulip_archive/stream/131828-t-compiler/topic/Review.20request.20for.20.2368241.html#187858177">(Feb 10 2020 at 21:07)</a>:</h4>
<p>If you like I can split out the block transfer function caching optimization and/or do another perf run</p>



<a name="187858285"></a>
<h4><a href="https://rust-lang.zulipchat.com#narrow/stream/131828-t-compiler/topic/Review%20request%20for%20%2368241/near/187858285" class="zl"><img src="https://rust-lang.github.io/zulip_archive/assets/img/zulip.svg" alt="view this post on Zulip" style="width:20px;height:20px;"></a> Dylan MacKenzie (ecstatic-morse) <a href="https://rust-lang.github.io/zulip_archive/stream/131828-t-compiler/topic/Review.20request.20for.20.2368241.html#187858285">(Feb 10 2020 at 21:08)</a>:</h4>
<p>Otherwise, we can always revert this if it actually causes a persistent wall-time regression. We'll be able to see it in the graphs.</p>



<a name="187876450"></a>
<h4><a href="https://rust-lang.zulipchat.com#narrow/stream/131828-t-compiler/topic/Review%20request%20for%20%2368241/near/187876450" class="zl"><img src="https://rust-lang.github.io/zulip_archive/assets/img/zulip.svg" alt="view this post on Zulip" style="width:20px;height:20px;"></a> pnkfelix <a href="https://rust-lang.github.io/zulip_archive/stream/131828-t-compiler/topic/Review.20request.20for.20.2368241.html#187876450">(Feb 11 2020 at 01:45)</a>:</h4>
<p>I mostly wanted clarity about your expectations</p>



<hr><p>Last updated: Aug 07 2021 at 22:04 UTC</p>
</html>